Refactor and Document check_env.check()#1826
Conversation
github-actions
Bot
added
python
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
github-actions
Bot
added
breaking-change
github-actions
Bot
added
non-breaking-change
github-actions
Bot
added
the
testing
github-actions
Bot
added
non-breaking-change
github-actions
Bot
added
the
dependencies
github-actions
Bot
removed
the
dependencies
|
|
||
| def check(env: str, field: str | None, optional: bool | None = False) -> str | None: | ||
|
|
||
| @overload |
There was a problem hiding this comment.
Overload is a type hinting feature.
It tells static type checkers that if you provide a certain type as an input you get a certain type as an output.
There was a problem hiding this comment.
Gotcha. Why are there three of them? It feels more verbose than is ideal (but maybe if we can use a comment to signal to the reader that these are type hints with no impact on the code itself, the verbosity is less of an issue)
There was a problem hiding this comment.
That's just how the syntax works, unfortunately.
That's why overload is only used situationally. I think it's worth it here since check is used in a ton of connectors so having accurate typing is extra useful.
There was a problem hiding this comment.
Happy to chat through how this works sometime, I just keep being busy on Thursdays 😅
It's a very helpful feature.
There was a problem hiding this comment.
You've got some other PRs I could review too so yeah let's schedule a call.
| Name of environment variable to check. | ||
| value: | ||
| If provided, ignore environment variable and return this. | ||
| opt: |
There was a problem hiding this comment.
Where is the opt parameter coming from? I get that it's being listed as deprecated, but I'm not seeing where it's used at all.
There was a problem hiding this comment.
optional (the former 3rd positional argument) is now a keyword-only argument. So I added "opt" as the 3rd positional argument with the deprecation warning. That way it's not a breaking change if people try to provide the optional parameter as a positional argument.
There was a problem hiding this comment.
Gotcha. What's your reasoning on moving it from a 3rd positional argument to a keyword-only argument?
There was a problem hiding this comment.
2 participants
What is this change?
check_env.check().optionalparameter to keyword-only.fieldparameter tovalue.optionalandfieldbreaking changes.Considerations for discussion
check_env.check()was minimally documented.How to test the changes (if needed)
Breaking Changes
Breaking changes are changes to our public API which may require existing users to change their code. If there are no breaking changes, any existing parsons user should not need to do anything after updating their parsons version.
Does this PR introduce breaking changes?